-
-
Notifications
You must be signed in to change notification settings - Fork 205
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
pack profiling nuget package #2800
Conversation
|
b6ab512
to
5f9c4ef
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks
edit: maybe add this to .craft.yml
in the same PR so it doesn't get forgotten?
<!-- TODO check and update the list of supported frameworks. --> | ||
<TargetFrameworks>net6.0</TargetFrameworks> | ||
<TargetFrameworks>net8.0;net6.0</TargetFrameworks> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Out of curiosity - why do we need to add net8 here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only 'strong' reason I can defend here is so it in this case is that if this package is added alone, it'll bring Sentry
as a transient dep. If we don't have net8.0 here, it'll pull net6.0 from Sentry
.
But wrt these decisions the mainstream idea is: In libs keep the lowest you need. But in reality there are some other cases where you want to bump (e.g ns2.0 is supported by net461 but causes issues due to rep resolution so years ago we had to ad net461 everywhere).
Recently on the .NET Discord someone from the .NET team was arguing against just saying "just keep the lowest" because we miss out on compiler improvements and other things added by the latest SDK when compiling to a newer target. Their take is to always support the latest version. Of course that has a cost so we can take that as a grain of salt.
This package specifically is something I hope we'll heavy invest, and will unblock new use cases for .NET folks using Sentry for performance monitoring. And since this is hopefully aligning with the core Sentry
package eventually (as in, merging into a single page) having this already on net8.0 made sense to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the thorough explanation, makes sense.
We can test the final package like this: https://github.com/getsentry/sentry-dotnet/blob/feat/4.0.0/integration-test/runtime.Tests.ps1 |
Cool. I tested locally for now so happy to merge if ya'll are OK with the change |
I'll keep it out for now since versioning is in tandem with Sentry and it requires pushing a new config in release registry. |
Testing bundling of the deps with in the profiling package
Microsoft.Diagnostics.NETCore.Client
to0.2.452401
net8.0
It works (as in, sends profiles to Sentry) but I get some long
unknown
instead of the functions I have spinning (the prime number examples from our samples):It's kind of fragile right now because the Sentry.Profiling project has a dependency on the perfview project. And then it manually adds 2 DLLs to the final package. I tested this by adding the nuget package via
PackageReference
to a test app and it worked fine. But ideally we'd have the other solution build outside of this project and only refer to the DLLs to get a behavior closer to what the consumers of the package will get.But this could unblock packaging nonetheless.
In the future, if this package turns out to work out well enough, we can put more effort into this, including moving it all under
Sentry
and droppingSentry.Profiling
altogether.